Trim trailing whitespace in cookie value: JavaNetCookieJar to avoid crash#9374
Conversation
vansh1sh
force-pushed
the
fix-cookiejar-trimmed-value-9373
branch
from
e3f2511 to
b0e8187
Compare
vansh1sh
force-pushed
the
fix-cookiejar-trimmed-value-9373
branch
from
b0e8187 to
da15e0e
Compare
vansh1sh
changed the title
yschimke
left a comment
yschimke
left a comment
There was a problem hiding this comment.
Looks safe enough, but would like to confirm both paths, via a Response with Set-Cookie and the path here via injecting directly into CookieHandler.
Is there a particular CookieHandler that has this issue?
We should fix anyway, but I'm curious if this is just a theoretical bug.
| } | ||
|
|
||
| @Test | ||
| fun cookieHandlerWithQuotedValueAndTrailingSpace() { |
There was a problem hiding this comment.
This is testing injecting a Cookie via CookieHandler. I can't reproduce via an actual request/response stored to the cookieJar, so I'm assuming this is only a problem with a shared or custom CookieHandler?
Can we add a test to confirm, something like
@Test
fun receiveAndSendUntrimmedCookie() {
server.enqueue(
MockResponse
.Builder()
.addHeader("Set-Cookie", "a=\"android \"")
.build(),
)
server.enqueue(MockResponse())
val cookieManager = CookieManager(null, CookiePolicy.ACCEPT_ORIGINAL_SERVER)
client =
client
.newBuilder()
.cookieJar(JavaNetCookieJar(cookieManager))
.build()
get(urlWithIpAddress(server, "/"))
val request1 = server.takeRequest()
assertThat(request1.headers["Cookie"]).isNull()
get(urlWithIpAddress(server, "/"))
val request2 = server.takeRequest()
assertThat(request2.headers["Cookie"]).isEqualTo("a=android")
}
There was a problem hiding this comment.
added a commit for this test. It succeeds in both the flows for me, so probably happening because of shared Cookie Handler.
|
@yschimke Thanks for the review! This is a production issue - we're seeing a significant number of crashes from this in our Android app (OkHttp 4.9.2). Tested locally with latest version and happening in that as well. Our CookieHandler wraps Android's android.webkit.CookieManager to share cookies between WebViews and native OkHttp requests. Now from my understanding, the WebKit stores the raw cookie value as-is from the server and getCookie() returns it without normalizing, unlike java.net.CookieManager which strips quotes during HttpCookie.parse(). So the whitespace inside quotes survives to decodeHeaderAsJavaNetCookies() and triggers the crash. This is a common pattern I've seen for Android app mixing WebView and native networking. Let me know if it's okay to merge this fix. I'll add your suggested test to cover both paths. |
vansh1sh
force-pushed
the
fix-cookiejar-trimmed-value-9373
branch
from
00603a8 to
654fcfc
Compare
|
@vansh1sh thanks, I basically wanted to understand if it was a wider problem. But that makes sense and is worth handling. |
2 participants
When JavaNetCookieJar receives a Cookie header from a CookieHandler with a quoted value that has trailing whitespace, for example:
Cookie: token="abc123 "
the value is unquoted to abc123 (with a trailing space), and Cookie.Builder.value currently throws IllegalArgumentException("value is not trimmed").
This PR makes decodeHeaderAsJavaNetCookies minimally tolerant of that case by trimming the value after unquoting:
It also adds a regression test to okhttp/src/jvmTest/kotlin/okhttp3/CookiesTest.kt
Fixes #9373.